Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v8.x] test: bump test/common to master #14459

Closed
wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

Backport one commit from #14011, perform a full copy of test/common to what’s in master and fix up tests accordingly (with the solutions that were used on master, too).

This is not pretty, but right now it seems like the most feasible solution to avoid future merge conflicts.

/cc @refack @BridgeAR

tniessen and others added 3 commits July 24, 2017 22:57
PR-URL: nodejs#14011
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
```
git checkout master test/common && \
    git commit -m 'test: bump test/common to master'
```
@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. v8.x labels Jul 24, 2017
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great.

@@ -23,7 +23,7 @@ assert.throws(
code: 'ERR_ASSERTION',
type: assert.AssertionError,
operator: undefined,
actual: undefined,
actual: 'custom message',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this semver-major?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not accompanied by any changes to lib or src, so it really can’t be. I have to admit I didn’t really figure out why this is required, if you like, feel free to do the digging.

Copy link
Contributor

@refack refack Jul 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember now. Before this change expectsError only checked ['message', 'type','code'] now it compares name if provided. And for AssertionError also ['generatedMessage', 'actual', 'expected', 'operator']

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so it sounds like everything is working as expected and better than before? :)

@@ -166,7 +166,7 @@ assert.throws(() => {
}, common.expectsError({ code: 'TEST_ERROR_1', type: RangeError }));
}, common.expectsError({
code: 'ERR_ASSERTION',
message: /^.+ is not the expected type \S/
message: /^.+ is not instance of \S/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not semver-major since it's a change in expectsError

addaleax pushed a commit to addaleax/node that referenced this pull request Jul 27, 2017
Backport-PR-URL: nodejs#14459
Backport-Reviewed-By: Refael Ackermann <[email protected]>

PR-URL: nodejs#14011
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Jul 27, 2017
```
git checkout master test/common && \
    git commit -m 'test: bump test/common to master'
```

PR-URL: nodejs#14459
Reviewed-By: Refael Ackermann <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Jul 27, 2017
@addaleax
Copy link
Member Author

Landed in 89586f6, d89bb1c, 1813467

@MylesBorins
Copy link
Contributor

should this be backported?

@MylesBorins
Copy link
Contributor

ping @addaleax

@addaleax
Copy link
Member Author

No, I don’t think this really can be backported. If we want, we can bump test/common on v6.x to master, but if that isn’t causing too much trouble it’s probably not worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants